Opentelemetry baggage propagation fix #1174
Conversation
|
|
|
Roman Konoval seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
romank0
force-pushed
the
baggage-propagation-fix
branch
2 times, most recently
from
3fd9236 to
411c6e3
Compare
There was a problem hiding this comment.
Two other things you'll have to do. First, poe lint is failing. Second, your commits have multiple email addresses associated:
You'll need to add the second email to your github account so that the CLA is signed for all the commits. Either that or modify the commits to the correct email in some way.
Change the activity interceptor to use context.attach()/detach() pattern instead of passing context as a parameter to start_as_current_span(). The fix follows the standard OpenTelemetry pattern used by other instrumentations (django, gRPC, etc.) and ensures proper context management with try/finally for detach.
Add additional tests to verify baggage propagation in scenarios: - multiple values - local activity - retries in activity
Two important edge case tests: - exceptions handling - when no current context is available
romank0
force-pushed
the
baggage-propagation-fix
branch
from
411c6e3 to
6675b3f
Compare
romank0
force-pushed
the
baggage-propagation-fix
branch
from
6675b3f to
0ee63e4
Compare
tests/contrib/test_opentelemetry.py
Outdated
| ) | ||
|
|
||
|
|
||
| EXPECT_FAILURE = True |
There was a problem hiding this comment.
What's the deal with this global? Are you just trying to put True and False in the parameters? If that's the case, just do that.
There was a problem hiding this comment.
That is just a way to make parameters in parametrized test more readable. To have this
[
(context_clear_noop_activity, not EXPECT_FAILURE),
(context_clear_exception_activity, EXPECT_FAILURE),
],
instead of this
[
(context_clear_noop_activity, not True),
(context_clear_exception_activity, True),
],
There was a problem hiding this comment.
Alternatively I can create an enum or even a dataclass for all parameters. I decided againts that as that is move verbose with very little gain IMHO
5 participants
What was changed
Fixed OpenTelemetry baggage propagation in the inbound interceptor by explicitly attaching the extracted context before starting spans. Changed from passing
context=extracted_ctxas a parameter to usingcontext.attach(extracted_ctx)+context.detach(token).Why?
The previous implementation used
start_as_current_span(context=extracted_ctx)which only uses the provided context to determine the parent span for trace propagation. When building the new span context, OpenTelemetry always usescontext.get_current()that is the active context from the stack, not thecontext=parameter. But the active context is not set from the unpacked values received in headers.This meant that while trace parent-child relationships worked correctly, baggage values from the extracted context were not copied into the new span context, making them unavailable within Temporal activities/workflows.
By calling
context.attach(extracted_ctx)first, we make the extracted context active on the context stack. This ensures that when the new span context is created, it copies all data (including baggage) from the extracted context, properly propagating baggage across service boundaries.This aligns with the standard pattern used by other OpenTelemetry instrumentations (django, gRPC, etc.).
Checklist
Closes [Feature Request] Make sure OTel baggage propagates properly throughout activities #362
How was this tested:
test_opentelemetry_baggage_propagation_basictest does this, namely:Any docs updates needed?
I don't think so.